-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: render debug merge-logs output in color #66629
cli: render debug merge-logs output in color #66629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @cameronnunez you might be interested in co-reviewing this?
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @postamar)
pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):
} err = log.FormatLegacyEntryPrefixTTY(prefixBytes, w) if err != nil {
in an ideal world, the debug merge-log
command would obey the customized output logging format for the stderr sink.
This way the command would output JSON if the user requested that. Right now the code (and your PR) enforces the output format be crdb-v1-tty
.
This approach will make-do for now, but we'll want to switch over around when #65633 merges.
pkg/util/log/format_crdb_v1.go, line 69 at r1 (raw file):
{ n := len(cp) counter := adler32.Checksum(prefix) % uint32(n-1)
I find this logic hard to read. What was wrong with simply prefixCode := adler32.Checksum(prefix) % uint32(len(cp)-1)
?
the "worst" that can happen is that some nodes do not get a color? But that's "a color" on its own too..
pkg/util/log/format_crdb_v1.go, line 88 at r1 (raw file):
return err } if _, err = w.Write([]byte("\033[7m")); err != nil {
it's odd to see a constant string inside this file. Maybe add the escape to the ttycolor package?
pkg/util/log/format_crdb_v1.go, line 97 at r1 (raw file):
} if prefixCode != ttycolor.Reset { if _, err = w.Write(cp[ttycolor.Reset]); err != nil {
please consider handling this with a defer
you can merge the errors with errors.CombineErrors()
.
Thanks for these comments! I'll address them in a couple of weeks when I'll be back. |
fa9ea54
to
4bad6a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):
Previously, knz (kena) wrote…
in an ideal world, the
debug merge-log
command would obey the customized output logging format for the stderr sink.
This way the command would output JSON if the user requested that. Right now the code (and your PR) enforces the output format becrdb-v1-tty
.This approach will make-do for now, but we'll want to switch over around when #65633 merges.
Understood.
pkg/util/log/format_crdb_v1.go, line 69 at r1 (raw file):
Previously, knz (kena) wrote…
I find this logic hard to read. What was wrong with simply
prefixCode := adler32.Checksum(prefix) % uint32(len(cp)-1)
?the "worst" that can happen is that some nodes do not get a color? But that's "a color" on its own too..
I simply didn't realize that ttycolor
was one that we maintained, I incorrectly assumed it was a third-party package. I submitted a pull request to ttycolor
for your review. Currently this PR doesn't build because the vendored version hasn't been updated yet (naturally).
pkg/util/log/format_crdb_v1.go, line 88 at r1 (raw file):
Previously, knz (kena) wrote…
it's odd to see a constant string inside this file. Maybe add the escape to the ttycolor package?
Done.
pkg/util/log/format_crdb_v1.go, line 97 at r1 (raw file):
Previously, knz (kena) wrote…
please consider handling this with a defer
you can merge the errors witherrors.CombineErrors()
.
Done.
40d23a9
to
ea708bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the two recommended changes
Reviewed 7 of 7 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @postamar)
pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):
Previously, postamar (Marius Posta) wrote…
Understood.
you can add a TODO in the code to that effect. Thanks
pkg/util/log/format_crdb.go, line 70 at r2 (raw file):
defer func() { _, errReset := w.Write(cp[ttycolor.Reset]) if errReset != nil {
no need for the conditional. CombineErrors already does this for you.
Previously, the log entries printed by the 'cockroach debug merge-logs' command were not rendered in color. This command takes log files, typically from a debug.zip, and merges them in a single stream. In so doing, it prefixes each entry with a string identifying its pre-merged origin on-disk. Typically, this is a node number. This commit renders the merged log entries in color using the existing legacy (v1) color scheme. Furthermore, the prefix backgrounds are also colored in a deterministic pseudorandom fashion to help identify the origin of each log entry. Release note (cli change): The 'cockroach debug merge-logs' command now renders in color by default.
ea708bd
to
8817257
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @knz)
pkg/cli/debug_merge_logs.go, line 65 at r1 (raw file):
Previously, knz (kena) wrote…
you can add a TODO in the code to that effect. Thanks
Done, thanks, I wasn't sure what you wanted me to do about this.
pkg/util/log/format_crdb.go, line 70 at r2 (raw file):
Previously, knz (kena) wrote…
no need for the conditional. CombineErrors already does this for you.
Neat!
bors r+ |
Build succeeded: |
Is there a way to disable this? It makes it really hard to look at the logs in an editor, with all of the ANSI code noise. I don't really think it makes sense to colorize the output when it's not going to a TTY, i.e. |
we don't typically add for changes on merged PRs. I'll file an issue. |
Already did: #69692. |
Previously, the log entries printed by the 'cockroach debug merge-logs'
command were not rendered in color. This command takes log files,
typically from a debug.zip, and merges them in a single stream. In so
doing, it prefixes each entry with a string identifying its pre-merged
origin on-disk. Typically, this is a node number.
This commit renders the merged log entries in color using the existing
legacy (v1) color scheme. Furthermore, the prefix backgrounds are also
colored in a deterministic pseudorandom fashion to help identify the
origin of each log entry.
Release note (cli change): The 'cockroach debug merge-logs' command now
renders in color by default.